Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve generated names for anonymous classes #5153

Closed
wants to merge 2 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Feb 6, 2020

In order of preference, the generated name will be:

new class extends ParentClass {};
// -> ParentClass@anonymous
new class implements FirstInterface, SecondInterface {};
// -> FirstInterface@anonymous
new class {};
// -> class@anonymous

This is intended to display a more useful class name in error messages and stack traces, and thus make debugging easier.

There is a minor backwards compatibility break here, in that code currently checking for a class@anonymous prefix should now check for @anonymous instead.

This change is based on the suggestion of @nicolas-grekas in #5143.

@PeeHaa
Copy link
Contributor

PeeHaa commented Feb 6, 2020

Isn't there a sane-ish way to somehow present both interfaces?

It seems pretty arbitrary to pick the first one.

@nicolas-grekas
Copy link
Contributor

I couldn't come up with a better proposal. Can you? Listing all interfaces can be pretty long very quickly, especially when considering namespaces. In practice, the first is often the most useful, so it's not that arbitrary to me.

@PeeHaa
Copy link
Contributor

PeeHaa commented Feb 6, 2020

I do agree it will get long fast. Not sure I have a better suggestion at this point besides somehow mashing the interfaces together with a delimiter.

In practice, the first is often the most useful

I don't understand why this would be the case. Unless I am missing your point the order of interfaces carries no weight one way or another.

@nikic
Copy link
Member Author

nikic commented Feb 6, 2020

I don't understand why this would be the case. Unless I am missing your point the order of interfaces carries no weight one way or another.

I think it's not so much that the first is the most useful, but that in the majority of the cases there will be only a single parent or a single interface, so this heuristic works well in practice.

We could of course include both the parent and all interfaces, and for that matter the file and line where it is declared, but ... that would make the name very long and unusable for error messages. We have to strike a reasonable compromise between providing relevant information and being concise here, and I think the approach proposed here is that compromise.

@mfn
Copy link
Contributor

mfn commented Feb 6, 2020

Here's an example of a project where I added tests where anonymous classes within tests are used: https://github.com/spatie/data-transfer-object/pull/81/files#diff-1e8fc8e188715be5ac191bc06e73ade9R30

I believe this one will break after this change (the anonymous class "extends" an actual class).

However this specific test already uses expectExceptionMessageRegExp so I guess I just need to adapt it and it's fine 👍

@PeeHaa
Copy link
Contributor

PeeHaa commented Feb 6, 2020

but that in the majority of the cases there will be only a single parent or a single interface, so this heuristic works well in practice

Which is totally fair as long as it is clear to people the arbitrary choice is made so that it actually is more clear instead of people getting confused (which I suspect would be the case when they first encounter it).

@guilliamxavier
Copy link
Contributor

Listing all interfaces can be pretty long very quickly, especially when considering namespaces

About that, could you please add a test with a namespaced base, just to make it clear whether e.g. \get_class(new class() extends \My\Ns\Foo {}) will be 'My\Ns\Foo@anonymous' . "\0" . <unique_stuff> or 'Foo@anonymous' . "\0" . <unique_stuff>?

In order of preference, the generated name will be:

    new class extends ParentClass {};
    // -> ParentClass@anonymous
    new class implements FirstInterface, SecondInterface {};
    // -> FirstInterface@anonymous
    new class {};
    // -> class@anonymous

This is intended to display a more useful class name in error messages
and stack traces, and thus make debugging easier.
@nikic
Copy link
Member Author

nikic commented Feb 7, 2020

@guilliamxavier I've added a test for that. The namespace is (of course) included.

@stollr
Copy link

stollr commented Feb 7, 2020

We could of course include both the parent and all interfaces, and for that matter the file and line

I would say that file and line would be most helpful for debugging purposes.

What happens if multiple anonymous classes extending the same parent class are declared one after the other?

@php-pulls php-pulls closed this in 72bd559 Feb 17, 2020
@nikic
Copy link
Member Author

nikic commented Feb 17, 2020

@Naitsirch FWIW, the file and line are included in the name: The full name (even after this change) is something like ParentClass@anonymous\0/some/long/path/file.php:42$123. Everything after the \0 is cut off in most contexts though, because the resulting name would be too unwieldy.

nicolas-grekas added a commit to symfony/polyfill that referenced this pull request Mar 3, 2020
This PR was merged into the 1.15-dev branch.

Discussion
----------

[Php80] Add get_debug_type()

Waiting for
- [ ] php/php-src#5143
- [ ] https://wiki.php.net/rfc/get_debug_type
- [x] php/php-src#5153

Commits
-------

61168ea [Php80] Add get_debug_type()
symfony-splitter pushed a commit to symfony/polyfill-php80 that referenced this pull request Mar 3, 2020
This PR was merged into the 1.15-dev branch.

Discussion
----------

[Php80] Add get_debug_type()

Waiting for
- [ ] php/php-src#5143
- [ ] https://wiki.php.net/rfc/get_debug_type
- [x] php/php-src#5153

Commits
-------

61168ea [Php80] Add get_debug_type()
@carusogabriel carusogabriel added this to the PHP 8.0 milestone May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants